Skip to content

Conversation

@elfkuzco
Copy link
Collaborator

@elfkuzco elfkuzco commented Oct 15, 2025

Changes

  • remove GET /offliners/{offliner_id} endpoint
  • add route to create offliner via API

This closes #1391

@elfkuzco elfkuzco self-assigned this Oct 15, 2025
@elfkuzco elfkuzco requested a review from benoit74 October 15, 2025 16:42
@elfkuzco
Copy link
Collaborator Author

@benoit74 , I am going to need your input on the permissions required for the offliner creation via API. I avoided using schedules, create=True because there are other roles with that permission that could not be prvileged to create an offliner. Left a todo comment on it. i don't know if this should be paused till #1419 is reviewed and possibly new permissions set.

WDYT?

@codecov
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.84%. Comparing base (7e4bd19) to head (720ae29).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1422      +/-   ##
==========================================
+ Coverage   82.74%   82.84%   +0.09%     
==========================================
  Files          79       79              
  Lines        3959     3952       -7     
  Branches      432      432              
==========================================
- Hits         3276     3274       -2     
+ Misses        564      559       -5     
  Partials      119      119              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

  • Regarding permissions, yes we need a new permission to create/update offliner definitions
  • I don't like the idea of getting a default offliner definition if version is not passed; this is very convenient in dev but very risky in production ; I feel like the version should be a mandatory parameter once WP1 and zimit-frontend have been migrated to pass this parameter ; and in this PR we assume it is

@elfkuzco
Copy link
Collaborator Author

WP1 does not really have a UI to "select the version" they want to run and I figured they would also want to do similar thing. Because the API returns the versions as a list of strings in descending order of created_at. So, if they are going to select the version they would use by "choosing the first" because it should be the most recent, I figured it would be simpler to do it here and avoid one more API call. Same thing applies to Zimit frontend

Do you have an idea on how the selection should be done?

@benoit74
Copy link
Collaborator

You probably saw that after your comment, just for the reference: openzim/wp1#1024 (comment) (we will configure the version in WP1 config file)

@elfkuzco elfkuzco force-pushed the offliner-definitions-cleanup branch from 1983ac5 to 38342ba Compare October 16, 2025 10:22
@elfkuzco elfkuzco requested a review from benoit74 October 16, 2025 10:23
Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please do not merge before openzim/zimit-frontend#176 and openzim/wp1#1024 are merged / solved

Moving to draft to avoid accident ^^

@benoit74 benoit74 marked this pull request as draft October 16, 2025 11:22
@elfkuzco elfkuzco force-pushed the offliner-definitions-cleanup branch from bd14429 to 1e46bcc Compare October 20, 2025 15:27
@elfkuzco elfkuzco changed the title remove model queries in alembic migration remove route to get initial offliner definiton Oct 21, 2025
@elfkuzco elfkuzco force-pushed the offliner-definitions-cleanup branch from 1e46bcc to d88439a Compare October 21, 2025 09:06
@elfkuzco elfkuzco force-pushed the offliner-definitions-cleanup branch from d88439a to 720ae29 Compare October 21, 2025 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cleanup transition of Zimfarm "dependencies" to offliner definition

3 participants